Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kvserver: wire up replica_rac range controller factory #130197

Merged

Conversation

kvoli
Copy link
Collaborator

@kvoli kvoli commented Sep 5, 2024

RangeControllerFactoryis used to create newRangeControllers when a replica becomes the raft leader. Add the missing required fields in RangeControllerFactoryand wire up the factory to sit on theStore`.

The RangeControllerFactory is one per-store, with callers providing
range specific information when asking for a new RangeController, and
the factory retaining one per-node or per-store controller dependencies.

Resolves: #130185
Release note: None

@kvoli kvoli self-assigned this Sep 5, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@kvoli kvoli marked this pull request as ready for review September 5, 2024 22:51
@kvoli kvoli requested review from a team as code owners September 5, 2024 22:51
@kvoli kvoli requested a review from sumeerbhola September 5, 2024 22:52
@kvoli
Copy link
Collaborator Author

kvoli commented Sep 5, 2024

This is a bit rough but lmk what you think about the approach @sumeerbhola.

Copy link
Collaborator

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 6 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @kvoli)


pkg/kv/kvserver/kvflowcontrol/replica_rac2/processor.go line 1032 at r2 (raw file):

// New creates a new RangeController.
func (f RangeControllerFactoryImpl) New(
	ctx context.Context, state rangeControllerInitState,

so rangeControllerInitState can stay package-private because this method isn't really called by anyone outside?

@kvoli
Copy link
Collaborator Author

kvoli commented Sep 6, 2024

TYFTR!

Yes, the processor is the only caller of New, so it can stay private.

Extended CI failure is for an auto marked flaky test.

bors r=sumeerbhola

@craig
Copy link
Contributor

craig bot commented Sep 6, 2024

Merge conflict.

`RangeControllerFactory` is used to create new `RangeController`s when a
replica becomes the raft leader. Add the missing required fields in
`RangeControllerFactory` and wire up the factory to sit on the `Store`.

The `RangeControllerFactory` is one per-store, with callers providing
range specific information when asking for a new `RangeController`, and
the factory retaining one per-node or per-store controller dependencies.

Resolves: cockroachdb#130185
Release note: None
@kvoli kvoli force-pushed the 240905.rac-processor-controller-hookup branch from f8ff890 to 79139c3 Compare September 6, 2024 15:36
@kvoli
Copy link
Collaborator Author

kvoli commented Sep 6, 2024

Rebased to pick up the 1st commit on master.

@kvoli
Copy link
Collaborator Author

kvoli commented Sep 6, 2024

bors r=sumeerbhola

@craig
Copy link
Contributor

craig bot commented Sep 6, 2024

Build failed:

@kvoli
Copy link
Collaborator Author

kvoli commented Sep 6, 2024

The generated code check failed on github CI but passed on engflow. I'm going to re-queue without making any changes.

bors r=sumeerbhola

@craig craig bot merged commit 636fd60 into cockroachdb:master Sep 6, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kvflowcontrol: wire up the range controller and processor
3 participants